Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow forc deploy to submit transaction without waiting for commit confirmation #6294

Merged
merged 35 commits into from
Aug 14, 2024

Conversation

luisburigo
Copy link
Contributor

@luisburigo luisburigo commented Jul 24, 2024

Description

The current forc deploy command is designed to submit and await for transaction finalization as much this is a common use case, for delayed transactions, were transaction are submitted but can take time a lot of time to be included and finalized. For this use cases I have implement a suggestion --submit-only this will follow the current flow, but only submit the transaction.

Example use case for delayed transactions

In Bako Safe, as it is a multisig, it is necessary to have signatures from multiple accounts before sending the transaction to the network. To work together with forc deploy, a GraphQL Proxy has been developed that works with the Fuel provider to allow sending the transaction to our protocol.

forc deploy --node-url 'https://api.bako.global/v1/graphql' --default-signer --submit-only

Changes

  • Add a new deploy params --submit-only
  • Create a branch on the code to verify if the param is provided
  • Abstract create artifacts to a single function to avoid code repetition
  • Modified transaction submission to allow for immediate submission when the submit_only command is provided
  • Implemented tests for the new command

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@CLAassistant
Copy link

CLAassistant commented Jul 24, 2024

CLA assistant check
All committers have signed the CLA.

@luisburigo luisburigo marked this pull request as ready for review July 24, 2024 21:04
@luisburigo luisburigo requested a review from a team as a code owner July 24, 2024 21:04
Copy link
Member

@sdankel sdankel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR! The use case makes sense, and the changes look look to me. Just a few nits.

Heads up, there will be som merge conflicts with #6069 and #6278 if they get merged first.

forc-plugins/forc-client/src/cmd/deploy.rs Outdated Show resolved Hide resolved
forc-plugins/forc-client/src/op/deploy.rs Outdated Show resolved Hide resolved
forc-plugins/forc-client/src/op/deploy.rs Outdated Show resolved Hide resolved
forc-plugins/forc-client/src/op/deploy.rs Outdated Show resolved Hide resolved
@luisburigo
Copy link
Contributor Author

Hey @sdankel , is it necessary to include --submit-only in the forc deploy documentation? If so, which file should I add it to?

@luisburigo luisburigo requested a review from sdankel July 25, 2024 14:37
@luizstacio luizstacio requested a review from a team July 26, 2024 12:59
@sdankel
Copy link
Member

sdankel commented Aug 1, 2024

Hey @sdankel , is it necessary to include --submit-only in the forc deploy documentation? If so, which file should I add it to?

Hey @luisburigo - good call out, and yes we should include it in the docs here. It would be great if you could add a short section at the end about multisig with --submit-only.

Copy link
Member

@sdankel sdankel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit about the documentation - otherwise LGTM :)

@luisburigo luisburigo requested a review from sdankel August 6, 2024 10:59
Copy link
Member

@K1-R1 K1-R1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, but CI is failing on spellchecking the word multisig; multisig needs to be added into the list in docs/book/spell-check-custom-words.txt

@luisburigo
Copy link
Contributor Author

lgtm, but CI is failing on spellchecking the word multisig; multisig needs to be added into the list in docs/book/spell-check-custom-words.txt

done ✅

sdankel
sdankel previously approved these changes Aug 8, 2024
Copy link
Member

@sdankel sdankel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@K1-R1 K1-R1 self-requested a review August 9, 2024 12:36
K1-R1
K1-R1 previously approved these changes Aug 9, 2024
Copy link
Member

@K1-R1 K1-R1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@luisburigo luisburigo dismissed stale reviews from K1-R1 and sdankel via fcf2029 August 9, 2024 16:11
@sdankel sdankel merged commit 9b87126 into FuelLabs:master Aug 14, 2024
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants